Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DeployAccount Method to account/account.go for Issue #381 #445

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Gabulhas
Copy link

Introduces a new DeployAccount method in account/account.go, aiming to simplify the process of account deployment on StarkNet as outlined in Issue #381. The DeployAccount method encapsulates multiple functions into a single call, making it easier and more intuitive for users to deploy accounts.

TODO:

  • Add tests (I don't know if this is needed)
  • Let users deploy accounts using the default UDC

@cicr99
Copy link
Contributor

cicr99 commented Oct 26, 2023

Thanks for this contribution! I see it still has a [WIP] at the beginning of the title, does this mean it still not ready for review? Can we change it into a draft if that's the case? Do you want us to take a look now even if it still not ready?

@Gabulhas
Copy link
Author

Gabulhas commented Nov 4, 2023

Sorry, was out for a conference for the last week

I will work on it more, I'll request for a review after :)

@Gabulhas
Copy link
Author

Gabulhas commented Nov 7, 2023

it's something really small by the way 👍

Can you review it?

@Gabulhas Gabulhas changed the title [WIP] Add DeployAccount Method to account/account.go for Issue #381 Add DeployAccount Method to account/account.go for Issue #381 Nov 8, 2023
account/account.go Outdated Show resolved Hide resolved
account/account.go Outdated Show resolved Hide resolved
account/account.go Outdated Show resolved Hide resolved
account/account.go Outdated Show resolved Hide resolved
account/account_test.go Outdated Show resolved Hide resolved
account/account_test.go Outdated Show resolved Hide resolved
account/account_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@rianhughes rianhughes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left some comments.

…teAddDeployAccount

- Created a separate example for CreateAndExecuteAddDeployAccount
- Merged with upstream and updated CreateAndExecuteAddDeployAccount

(Fixed issues mentioned in NethermindEth/pull/445)
@Gabulhas
Copy link
Author

Just made the changes you asked, let me know if there's something more 😄

ConstructorCalldata []*felt.Felt // ConstructorCalldata contains the calldata to be passed to the constructor of the account contract upon deployment.
}

func (account *Account) CreateAndExecuteAddDeployAccount(options DeployOptions) (*rpc.AddDeployAccountTransactionResponse, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works for rpcv05 and lower. However, rpcv06 introduces a new deployAccount transaction version (v3), meaning users won't be able to use this method to submit v3 transactions. It would be great if we could make this compatible with both transaction versions (eg by accepting the transaction as an argument, tx rpc.DeployAccountType)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure will fix it

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by "this method" you mean the whole method "CreateAndExecuteAddDeployAccount" or the way you deploy accounts like in the function?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CreateAndExecuteAddDeployAccount method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants